Skip to content

Add count methods to ELC's ReactiveElasticsearchClient. #2985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 17, 2024
Merged

Add count methods to ELC's ReactiveElasticsearchClient. #2985

merged 1 commit into from
Oct 17, 2024

Conversation

maryantocinn
Copy link
Contributor

@maryantocinn maryantocinn commented Oct 11, 2024 •
edited
Loading

Please let me know if i've missed anything as this is my first time contributing to an open source project.

  • Implemented count methods using CountRequest directly with ELC's ReactiveElasticsearchClient, eliminating the need to initialize any entity or repository.
  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our issue tracker. Add the issue number to the Closes #issue-number line below
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

Closes #2749

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 11, 2024
sothawo
sothawo requested changes Oct 13, 2024
View reviewed changes
Copy link
Collaborator

@sothawo sothawo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR to add the count method to the ReactiveElasticsearchClient, this is all fine for the ReactiveElasticsearchClient, but we cannot use this in org.springframework.data.elasticsearch.client.elc.ReactiveElasticsearchTemplate#doCount - at least not without further investigation.

Why? Originally the doCount was implemented with a CountRequest( #341), the old one from the previous Elasticsearch libraries. We had to change this to a search query with a size of 0; you find a hint in this old issue: #1290, the count request does not consider the filters set on a query. or the diff in this old commit: 2cd1817#diff-8edbaefd20710bf00d7a492f1d368f3c9efbce5b4628afd4b43a72a8b28af17fL518-R294

Before going to a count request again, it would be necessary to investigate in that and see if it is safe to do this replacement.

But that I'd see in a diferent issue, and have this one just for adding the methods to the ReactiveElasticsearchClient.

@maryantocinn
Copy link
Contributor Author

I see, thank you for the context. I'll leave the investigation of reverting to a CountRequest for ReactiveElasticsearchTemplate#doCount for a separate issue as it will need a deeper analysis of how the filters work.

For now, I'll revert all the changes made to ReactiveElasticsearchTemplate and focus this PR on adding the count methods to the ReactiveElasticsearchClient as initially intended.

@maryantocinn maryantocinn force-pushed the #2749-add-count-method branch from 86c537a to b7d60ce Compare October 16, 2024 06:24
@maryantocinn
Add count methods to ELC's ReactiveElasticsearchClient.
6ba63ec 
Implemented count methods using CountRequest directly with ELC's ReactiveElasticsearchClient, eliminating the need to initialize any entity or repository.

Closes spring-projects#2749
@maryantocinn maryantocinn force-pushed the #2749-add-count-method branch from 74a139b to 6ba63ec Compare October 16, 2024 07:13
@sothawo sothawo merged commit fe8c2b1 into spring-projects:main Oct 17, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sothawo sothawo sothawo approved these changes

Assignees
No one assigned
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ReactiveElasticsearchClient missing simple count() method
3 participants
@maryantocinn @sothawo @spring-projects-issues